Skip to content

Add missing docs for System.Collections[.Generic] #4121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 29, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Apr 17, 2020

Contributes to #3914.

Adds docs for 1.x gaps not added in #4060 (which addressed System.Collections.Immutable)

@layomia layomia requested a review from carlossanlop April 17, 2020 05:45
@layomia layomia requested a review from eiriktsarpalis as a code owner April 17, 2020 05:45
@layomia layomia self-assigned this Apr 17, 2020
@dotnet-bot dotnet-bot added this to the April 2020 milestone Apr 17, 2020
@carlossanlop carlossanlop requested a review from a team April 17, 2020 19:32
@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles Release: .NET Core 1.x labels Apr 17, 2020
@carlossanlop
Copy link
Contributor

Consider mentioning any expected exceptions as well. Can you please check if any of these APIs throw?

@layomia
Copy link
Contributor Author

layomia commented Apr 20, 2020

Consider mentioning any expected exceptions as well. Can you please check if any of these APIs throw?

Hmm weird, I made triple-slash additions in the source locally so I could use the porting tool, but somehow the exceptions weren't picked up. I'll hand-write them and push a commit.

@layomia layomia requested a review from carlossanlop April 21, 2020 02:19
@carlossanlop
Copy link
Contributor

carlossanlop commented Apr 22, 2020

Hmm weird, I made triple-slash additions in the source locally so I could use the porting tool, but somehow the exceptions weren't picked up.

@layomia Exceptions don't get picked up by default by the tool because there is no easy way to determine if an exception message has been added or not (usually the message gets modified during language review, making it impossible to compare with the original message).

To enable the option, add the argument -skipExceptions false to the execution command, then run it again against this namespace.

Note: I fixed a bug in the tool today in the exception porting code. Please pull the latest changes to your machine.

@layomia
Copy link
Contributor Author

layomia commented Apr 22, 2020

@carlossanlop, I've added the exceptions - PTAL.

<remarks>To be added.</remarks>
<exception cref="System.ArgumentException"><paramref name="collection" /> contains one or more duplicated keys.</exception>
Copy link
Contributor

@carlossanlop carlossanlop Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @layomia did you manually add the exceptions or did you port them with the tool? I ask because the exception cref DocIDs are incomplete (missing the API category prefix), and if it's the tool's fault, I should get it fixed.

This is how it should look:

Suggested change
<exception cref="System.ArgumentException"><paramref name="collection" /> contains one or more duplicated keys.</exception>
<exception cref="T:System.ArgumentException">
<paramref name="collection" /> contains one or more duplicated keys.</exception>

Also, if the inner value of the <exception> starts with an xml tag, we usually prefer to start the text with an endline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added them manually in e42c4c8, but used the tool to port the in the new commit. It missed the one added in Queue1.xml (not sure how), so I applied your suggestion in e0f79cf and rebased.

The command I used was:

DocsPortingTool -docs D:\repos\dotnet-api-docs\xml -tripleslash D:\repos\dotnet_runtime\artifacts\bin\coreclr\Windows_NT.x64.Release\IL\,D:\repos\dotnet_runtime\artifacts\bin\ -includedassemblies System.Collections,System.Collections.Generic,mscorlib -excludedassemblies System.Collections.Immutable,System.Collections.Specialized,System.Collections.Concurrent -save true -skipExceptions false

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the exceptions, @layomia. Can you please take a look at my suggestions and the question I asked on the first one?

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @layomia.

@carlossanlop carlossanlop merged commit 95591fd into dotnet:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants